Skip to content

[PM-39361] fix: Restart card scanner after camera interruption or reopen#2847

Open
fedemkr wants to merge 3 commits into
mainfrom
PM-39361/fix-card-scanner-unresponsive
Open

[PM-39361] fix: Restart card scanner after camera interruption or reopen#2847
fedemkr wants to merge 3 commits into
mainfrom
PM-39361/fix-card-scanner-unresponsive

Conversation

@fedemkr

@fedemkr fedemkr commented Jun 30, 2026

Copy link
Copy Markdown
Member

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-39361

📔 Objective

Fixes a blank camera screen when opening the card scanner after the Add Card view was closed while the scanner was still open, or after the device locks and unlocks while the app is on the Add Card screen.

Changes in CardScannerView.swift:

  • Replace try? startScanning() with explicit error handling; on failure a new
    onScannerUnavailable callback is fired (dispatched after the current SwiftUI
    update pass to avoid re-entrant state mutations).
  • Implement dataScanner(_:becameUnavailableWithError:) in the Coordinator to
    forward runtime camera-interruption events through the same callback.
  • Add @Environment(\.scenePhase) observation in CardScannerWrapperView; when
    the scene becomes .active while scanning is supposed to be running, a
    stop-then-restart cycle is triggered.
  • Retry up to 2 times with a 300 ms delay before auto-dismissing the sheet, so
    the user is never left with a permanently blank screen.

@fedemkr fedemkr added the ai-review Request a Claude code review label Jun 30, 2026
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug labels Jun 30, 2026
@fedemkr
fedemkr marked this pull request as ready for review June 30, 2026 21:48
@fedemkr
fedemkr requested review from a team and matt-livefront as code owners June 30, 2026 21:48
@fedemkr fedemkr added the enhancement New feature or request label Jun 30, 2026
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the card scanner restart logic in CardScannerView.swift and its new Coordinator unit tests. The latest commit addresses the two concerns raised earlier in this PR: the retry-budget double-counting (foreground resumes now use a separate resumeScanningAfterForeground() path that does not consume the failure budget) and the scenePhase unreliability under UIKit hosting (replaced with a UIApplication.willEnterForegroundNotification observer). The retry cap, dispatch-after-update-pass handling of startScanning() failures, and scanner lifecycle in AddEditCardItemView are all coherent and within scope.

Code Review Details

No new blocking findings. The retry-budget and scene-phase issues flagged earlier in this PR have been resolved by the latest commit. One open human review thread (CardScannerView.swift:172 regarding the DispatchQueue.main.async rationale) remains for the author to answer directly.

Comment on lines 65 to 87
.onChange(of: scenePhase) { newPhase in
if newPhase == .active, isScanning {
restartScanning()
}
}
}

// MARK: Private

/// Stops scanning, waits 300 ms for the camera to fully release, then restarts.
/// After two retries the sheet is dismissed so the user is never left with a blank screen.
private func restartScanning() {
guard scannerRetryCount < 2 else {
dismiss()
return
}
scannerRetryCount += 1
isScanning = false
Task { @MainActor in
try? await Task.sleep(for: .milliseconds(300))
isScanning = true
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ IMPORTANT: Benign foreground transitions consume the retry budget and can auto-dismiss a working scanner.

Details and fix

scannerRetryCount is shared between two very different triggers: genuine scanner failures (onScannerUnavailable) and routine scenePhase == .active transitions (line 66). It only resets on .onAppear, which does not re-fire when the app is backgrounded and foregrounded while the sheet stays presented.

Trace:

After two normal app-switches the sheet closes on the next foreground even though the camera is fine. A successful restart also never decrements the counter, so each recovery permanently shrinks the budget available for a later real failure.

Consider resetting scannerRetryCount = 0 on a successful restart (e.g. after isScanning = true settles), or only counting onScannerUnavailable-driven restarts toward the cap rather than scene-phase restarts.

do {
try uiViewController.startScanning()
} catch {
DispatchQueue.main.async { context.coordinator.onScannerUnavailable?() }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Is the dispatch here just to avoid updating the view in the middle of a view update?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed.

Comment on lines +65 to +67
.onChange(of: scenePhase) { newPhase in
if newPhase == .active, isScanning {
restartScanning()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Does the change of scenePhase trigger for you? I think the scene phase might only work if you're using SwiftUI navigation, not UIKit. You might need to observe the notification center notifications instead.

fedemkr added 2 commits July 17, 2026 13:00
…annerView

Replace scenePhase (unreliable in this UIKit-hosted app) with a
UIApplication.willEnterForegroundNotification observer, and stop
foreground-triggered restarts from consuming the same retry budget
as genuine scanner failures.
@fedemkr
fedemkr requested a review from matt-livefront July 17, 2026 19:37
.onChange(of: scenePhase) { newPhase in
if newPhase == .active, isScanning {
restartScanning()
.onReceive(NotificationCenter.default.publisher(for: UIApplication.willEnterForegroundNotification)) { _ in

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matt-livefront I wasn't sure whether this would be a good idea to put it directly here. It seems more like SwiftUI but less testable so if needed I can move it to the processor and do the logic somehow there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review app:password-manager Bitwarden Password Manager app context enhancement New feature or request t:bug Change Type - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants